-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ArrayOps
bugs (by avoiding ArraySeq#array
, which does not guarantee element type)
#9641
Conversation
On the face of it, this looks like a quick merge. But (after discussion with Dale) I actually want to slow down and understand if a better fix is possible. According to the Scaladoc for
so if Must |
in
this seems incorrect to me -- it assumes behavior from one thing I don't understand here yet is why |
maybe the @scala/collections crew would like to pile on, here |
I am inclined to agree with Seth that the thing that needs fixing is the |
Yes, my comment was it fixes the issue "in a way" because there are alternatives and the quick fix is not compelling. ArrayOps says "The remaining methods are provided for completeness but they delegate to mutable.ArraySeq implementations which may not provide the best possible performance." So all those calls could rebuild the desired array as necessary without violating the lightbend support contract. The original sin is
#6648
Here is the unused (
|
First, is it important to do the Second, the ArrayOps intersect implementation should be rewritten to avoid the cast IMO. The cast is assuming something is too strong. I think something as simple as: val bs = that.toSet
val resultSize = count(bs)
val result = new Array[A](resultSize)
var idx = 0
var cnt = 0
while (cnt < resultSize) {
val a = apply(idx)
if (bs(a)) {
result(cnt) = a
cnt += 1
}
idx += 1
}
result Third, the change in this PR also seems worthwhile to me, except I think we should weaken the tests so that we aren't testing the classtypes (since I don't think that is part of the signature). |
Thanks, I'll do a quickie PR with re-building the result in ArrayOps as necessary. I don't mind doing it as an exercise, if that is also not desirable. |
I don't have a knowledgeable opinion about empty and caching it. Let me add that "knowledgeable" is a very strange word. What does it even mean? |
It was less sinful originally when it was
Yes and no? fdb1e69 states it avoids allocating 100+ million empty arrays but at the same time how it didn't make as much as a difference as it sounds like it should. 🤷🏼♂️ |
please note that the problematic method is |
thoughts about adding a private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM otherwise, arraySeq.array
is doing what it says, so such workarounds are needed if we want to use it.
I'm also fine if someone wants to come up with a solution that doesn't use ArraySeq
.
I simplified to just do @johnynek I leveraged the comment about "these are for completeness not efficiency". @NthPortal 's last comment went over my head, so I'll leave that for the collections collective. |
Yes, I think so, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this PR is now mergeable as-is. Along the way some questions and suggestions came up about possible future PRs in the same area, but this PR seems like the right minimal fix to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change the title of the PR to "Avoid using ArraySeq#array inside of ArrayOps" or something similar.
ArrayOps
bugs (by avoiding using ArraySeq#array
, which does not guarantee element type)
what's the story with changes like this wrt to scala 3? I guess it has the same bug, right? |
Periodically scala 3 bumps their scala-library version. So the Scala 3 release after 2.13.7 will have this. |
ArrayOps
bugs (by avoiding using ArraySeq#array
, which does not guarantee element type)ArrayOps
bugs (by avoiding ArraySeq#array
, which does not guarantee element type)
In a way,
Fixes scala/bug#12403
(which regressed in #9365)